Skip to content

Conversation

@matt-aitken
Copy link
Member

This is currently used in the dashboard only

@changeset-bot
Copy link

changeset-bot bot commented Oct 1, 2025

⚠️ No Changeset found

Latest commit: e6727c7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Walkthrough

Tag fetching moved from project-scoped to environment-scoped. Frontend: RunFilters/TagsDropdown now resolve environment context and include time filters (period/from/to) in tag fetches; name query params are no longer URL-encoded and useSearchParams stops decoding values. Backend: added an environment-scoped loader at routes/resources.environments.$envId.runs.tags.tsx; RunTagListPresenter.call signature now requires organizationId and environmentId and accepts period/from/to. RunsRepository gained listTags with Postgres and ClickHouse implementations; ClickHouse package exposes a new tag query builder. The old project-scoped tags loader was removed. tsconfig.check.json excludes test files.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Changes span frontend components, hooks, route loaders, presenter API, repository interface and implementations, and internal ClickHouse code
  • Heterogeneous edits requiring review of parameter validation, time-filter propagation, encoding/decoding semantics, pagination, and query correctness
  • Coordination needed to verify updated function signatures and removal of the old project-scoped route across layers

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description does not follow the repository’s required template and is missing all sections such as the issue reference, checklist, testing steps, changelog, and screenshots. Please update the description to include the “Closes #” statement, complete the ✅ Checklist, describe testing steps under a Testing section, add a brief Changelog entry, and include any relevant screenshots.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Tags listing now uses ClickHouse” clearly summarizes the primary change of switching the tags listing implementation to ClickHouse, matching the author’s stated objective and focusing on the main update without unnecessary detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tags-list-performance

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)

838-852: Stop double encoding the tag search value.

Line 840 wraps searchValue in encodeURIComponent, but URLSearchParams already encodes for us. A query like “foo bar” becomes foo%2520bar, and the loader only decodes once, so the presenter receives foo%20bar and the ClickHouse search fails to match real tag names. Remove the manual encoding and let URLSearchParams handle it.

-    if (searchValue) {
-      searchParams.set("name", encodeURIComponent(searchValue));
-    }
+    if (searchValue) {
+      searchParams.set("name", searchValue);
+    }
🧹 Nitpick comments (1)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)

316-332: Missing fallback logic for resilience.

Unlike listRunIds, listRuns, and countRuns (lines 184-314), the listTags method does not include fallback logic to retry with Postgres if ClickHouse fails. This creates an inconsistency in resilience patterns.

Apply this diff to add fallback logic:

   async listTags(options: TagListOptions): Promise<TagList> {
     const repository = await this.#getRepository();
     return startActiveSpan(
       "runsRepository.listTags",
       async () => {
-        return await repository.listTags(options);
+        try {
+          return await repository.listTags(options);
+        } catch (error) {
+          // If ClickHouse fails, retry with Postgres
+          if (repository.name === "clickhouse") {
+            this.logger?.warn("ClickHouse failed, retrying with Postgres", { error });
+            return startActiveSpan(
+              "runsRepository.listTags.fallback",
+              async () => {
+                return await this.postgresRunsRepository.listTags(options);
+              },
+              {
+                attributes: {
+                  "repository.name": "postgres",
+                  "fallback.reason": "clickhouse_error",
+                  "fallback.error": error instanceof Error ? error.message : String(error),
+                  organizationId: options.organizationId,
+                  projectId: options.projectId,
+                  environmentId: options.environmentId,
+                },
+              }
+            );
+          }
+          throw error;
+        }
       },
       {
         attributes: {
           "repository.name": repository.name,
           organizationId: options.organizationId,
           projectId: options.projectId,
           environmentId: options.environmentId,
         },
       }
     );
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0597691 and 6496171.

📒 Files selected for processing (11)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (6 hunks)
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (2 hunks)
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (1 hunks)
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (1 hunks)
  • apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx (0 hunks)
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (2 hunks)
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts (2 hunks)
  • apps/webapp/app/services/runsRepository/runsRepository.server.ts (4 hunks)
  • apps/webapp/tsconfig.check.json (1 hunks)
  • internal-packages/clickhouse/src/index.ts (2 hunks)
  • internal-packages/clickhouse/src/taskRuns.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/clickhouse/src/taskRuns.ts
  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • internal-packages/clickhouse/src/index.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
🧠 Learnings (1)
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.

Applied to files:

  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
🧬 Code graph analysis (9)
internal-packages/clickhouse/src/taskRuns.ts (1)
internal-packages/clickhouse/src/client/types.ts (1)
  • ClickhouseReader (20-75)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (2)
  • TagListOptions (7-19)
  • TagList (23-23)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskRuns.ts (1)
  • getTaskRunTagsQueryBuilder (118-127)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (1)
packages/trigger-sdk/src/v3/tags.ts (1)
  • tags (12-14)
apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • TagListOptions (7-19)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
  • TagListOptions (112-121)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (3)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (3)
  • TagListOptions (112-121)
  • name (158-160)
  • RunsRepository (141-333)
apps/webapp/app/services/clickhouseInstance.server.ts (1)
  • clickhouseClient (5-5)
apps/webapp/app/db.server.ts (1)
  • PrismaClient (370-370)
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • TagListOptions (7-19)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
  • TagListOptions (112-121)
apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (4)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/db.server.ts (1)
  • $replica (103-106)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • RunTagListPresenter (26-64)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
  • useEnvironment (19-23)
apps/webapp/app/hooks/useSearchParam.ts (1)
  • useSearchParams (7-70)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/tsconfig.check.json (1)

12-13: LGTM!

The test file exclusion pattern is appropriate and will prevent type-checking overhead on test files during the check process.

apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (2)

68-75: LGTM!

The shift to environment-scoped identifiers and addition of a 1-year time filter appropriately scopes tag queries to the current environment context.


77-77: Verified response structure RunTagListPresenter.call returns tags as a string[], so replacing tags.tags.map(t => t.name) with tags.tags is correct.

apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)

72-75: LGTM!

Clean pagination type for offset-based queries.

Comment on lines +112 to +125
export type TagListOptions = {
organizationId: string;
projectId: string;
environmentId: string;
period?: string;
from?: number;
to?: number;
/** Performs a case insensitive contains search on the tag name */
query?: string;
} & OffsetPagination;

export type TagList = {
tags: string[];
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Type mismatch with presenter and misleading documentation.

Issues identified:

  1. Type mismatch: TagListOptions.from and to are typed as number (timestamps), but RunTagListPresenter.server.ts (line 6-18 in relevant snippets) uses Date objects. This will cause type errors or runtime issues.

  2. Misleading documentation: Line 119 says "Performs a case insensitive contains search" but the Postgres implementation (postgresRunsRepository.server.ts) uses startsWith filtering, not contains.

Apply this diff to align types:

 export type TagListOptions = {
   organizationId: string;
   projectId: string;
   environmentId: string;
   period?: string;
-  from?: number;
-  to?: number;
+  /** Unix timestamp in milliseconds */
+  from?: number;
+  /** Unix timestamp in milliseconds */
+  to?: number;
   /** Performs a case insensitive contains search on the tag name */
   query?: string;
 } & OffsetPagination;

Also ensure the presenter converts Date objects to timestamps before calling the repository, or change the repository to accept Date objects.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export type TagListOptions = {
organizationId: string;
projectId: string;
environmentId: string;
period?: string;
from?: number;
to?: number;
/** Performs a case insensitive contains search on the tag name */
query?: string;
} & OffsetPagination;
export type TagList = {
tags: string[];
};
export type TagListOptions = {
organizationId: string;
projectId: string;
environmentId: string;
period?: string;
/** Unix timestamp in milliseconds */
from?: number;
/** Unix timestamp in milliseconds */
to?: number;
/** Performs a case insensitive contains search on the tag name */
query?: string;
} & OffsetPagination;
export type TagList = {
tags: string[];
};
🤖 Prompt for AI Agents
In apps/webapp/app/services/runsRepository/runsRepository.server.ts around lines
112-125, update TagListOptions.by changing from?: number and to?: number to
from?: Date and to?: Date to match RunTagListPresenter.server.ts; change the
JSDoc for query to say "Performs a case-insensitive startsWith (prefix) search"
to reflect the Postgres implementation; then update the Postgres repository
implementation (postgresRunsRepository.server.ts) to accept Date values and
internally convert them to timestamps (e.g., .getTime()) when building DB
queries so types are consistent end-to-end.

@matt-aitken matt-aitken force-pushed the tags-list-performance branch from 6496171 to 7135b71 Compare October 14, 2025 14:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)

836-852: Include environment.id in dependencies and avoid double-encoding name.

  • Add environment.id to the effect deps to refetch when env changes.
  • Let URLSearchParams handle encoding; remove manual encode/decode pair.
-  useEffect(() => {
+  useEffect(() => {
     const searchParams = new URLSearchParams();
-    if (searchValue) {
-      searchParams.set("name", encodeURIComponent(searchValue));
-    }
+    if (searchValue) searchParams.set("name", searchValue);
     if (period) {
       searchParams.set("period", period);
     }
     if (from) {
       searchParams.set("from", from.getTime().toString());
     }
     if (to) {
       searchParams.set("to", to.getTime().toString());
     }
-    fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
-  }, [searchValue, period, from?.getTime(), to?.getTime()]);
+    fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
+  }, [environment.id, searchValue, period, from?.getTime(), to?.getTime()]);
♻️ Duplicate comments (3)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (2)

112-121: Clarify time types and align docs with downstream usage.

from/to are numbers; add docs to state Unix ms. Also ensure Postgres implements the same "contains" semantics as ClickHouse, or update this comment to "prefix" to match reality.

Apply this diff:

 export type TagListOptions = {
   organizationId: string;
   projectId: string;
   environmentId: string;
   period?: string;
-  from?: number;
-  to?: number;
-  /** Performs a case insensitive contains search on the tag name */
+  /** Unix timestamp in milliseconds */
+  from?: number;
+  /** Unix timestamp in milliseconds */
+  to?: number;
+  /** Performs a case-insensitive contains search on the tag name */
   query?: string;
 } & OffsetPagination;

138-138: Enforce missing filters in PostgresRunsRepository.listTags
The method only applies projectId, query, offset, and limit. Update the Prisma findMany call to also filter by organizationId, environmentId, and period/from/to as defined in TagListOptions.
apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts:108

apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (1)

205-207: Offset is ignored; pagination returns wrong page.

listTags applies limit but not offset. Presenter passes offset=(page−1)*pageSize; without offset, every page loads from the start.

-    // Add ordering and pagination
-    queryBuilder.orderBy("tag ASC").limit(options.limit);
+    // Add ordering and pagination
+    queryBuilder.orderBy("tag ASC");
+    if (options.limit !== undefined) {
+      queryBuilder.limit(options.limit, options.offset ?? 0);
+    }
🧹 Nitpick comments (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)

5-5: Remove unused import.

The timeFilters import is not used anywhere in this file.

Apply this diff to remove the unused import:

-import { timeFilters } from "~/components/runs/v3/SharedFilters";
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)

836-852: Optional: debounce tag fetches to reduce request volume while typing.

Use useDebounceEffect (as with queues/versions) for TagsDropdown fetch.

Example:

-  useEffect(() => {
-    // build searchParams...
-    fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
-  }, [environment.id, searchValue, period, from?.getTime(), to?.getTime()]);
+  useDebounceEffect(
+    [environment.id, searchValue, period, from?.getTime(), to?.getTime()].join("|"),
+    () => {
+      const searchParams = new URLSearchParams();
+      if (searchValue) searchParams.set("name", searchValue);
+      if (period) searchParams.set("period", period);
+      if (from) searchParams.set("from", from.getTime().toString());
+      if (to) searchParams.set("to", to.getTime().toString());
+      fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
+    },
+    200
+  );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6496171 and 7135b71.

📒 Files selected for processing (11)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (5 hunks)
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (2 hunks)
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (1 hunks)
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx (1 hunks)
  • apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx (0 hunks)
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (2 hunks)
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts (2 hunks)
  • apps/webapp/app/services/runsRepository/runsRepository.server.ts (4 hunks)
  • apps/webapp/tsconfig.check.json (1 hunks)
  • internal-packages/clickhouse/src/index.ts (2 hunks)
  • internal-packages/clickhouse/src/taskRuns.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/webapp/app/routes/resources.projects.$projectParam.runs.tags.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
  • apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx
  • apps/webapp/app/services/runsRepository/postgresRunsRepository.server.ts
  • internal-packages/clickhouse/src/taskRuns.ts
  • apps/webapp/tsconfig.check.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/clickhouse/src/index.ts
  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/services/runsRepository/runsRepository.server.ts
  • apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts
  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
🧠 Learnings (1)
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.

Applied to files:

  • apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts
🧬 Code graph analysis (6)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskRuns.ts (1)
  • getTaskRunTagsQueryBuilder (124-131)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (2)
  • TagListOptions (7-19)
  • TagList (23-23)
apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (2)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • TagListOptions (7-19)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (1)
  • TagListOptions (112-121)
apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (4)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/db.server.ts (1)
  • $replica (103-106)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • RunTagListPresenter (26-64)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (3)
apps/webapp/app/services/runsRepository/runsRepository.server.ts (3)
  • TagListOptions (112-121)
  • name (158-160)
  • RunsRepository (141-333)
apps/webapp/app/services/clickhouseInstance.server.ts (1)
  • clickhouseClient (5-5)
apps/webapp/app/db.server.ts (1)
  • PrismaClient (370-370)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
  • useEnvironment (19-23)
apps/webapp/app/hooks/useSearchParam.ts (1)
  • useSearchParams (7-70)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
internal-packages/clickhouse/src/index.ts (1)

154-161: Wiring for tagQueryBuilder looks correct.

Consistent with other builders; no issues spotted.

apps/webapp/app/services/runsRepository/clickhouseRunsRepository.server.ts (1)

198-203: Case-insensitive contains filter is fine; consider parameterizing with wildcards only if switching to LIKE.

Current positionCaseInsensitiveUTF8(tag, {query}) > 0 matches contains semantics and aligns with docs. If you ever switch to LIKE, remember to wrap with %query%.

apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (1)

19-67: Loader auth, validation, and time filter normalization look good.

Access is scoped to org membership; time filters normalized; presenter called with correct IDs and types.

Please confirm presenter TagListOptions vs repository TagListOptions type mismatch is intentional (presenter uses Date, repository expects ms number). It works now due to conversion; consider aligning types in a follow-up.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx (1)

330-338: Minor: avoid trailing “?” and stale deps in effect

  • The template always appends “?” even when there are no params.
  • Effect reads organization/project/environment/fetcher but doesn’t include them in deps.

Optional tweak for clarity and correctness:

   useEffect(() => {
-    const searchParams = new URLSearchParams();
-    if (searchValue) {
-      searchParams.set("name", searchValue);
-    }
-    fetcher.load(
-      `/resources/orgs/${organization.slug}/projects/${project.slug}/env/${environment.slug}/waitpoints/tags?${searchParams}`
-    );
-  }, [searchValue]);
+    const sp = new URLSearchParams();
+    if (searchValue) sp.set("name", searchValue);
+    const qs = sp.toString();
+    const url = `/resources/orgs/${organization.slug}/projects/${project.slug}/env/${environment.slug}/waitpoints/tags${qs ? `?${qs}` : ""}`;
+    fetcher.load(url);
+  }, [searchValue, organization.slug, project.slug, environment.slug, fetcher]);
apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (3)

36-44: Trim name and keep empty-as-undefined

Avoid passing whitespace-only names downstream. Trim and normalize to undefined.

-  const parsedSearchParams = SearchParams.safeParse({
+  const parsedSearchParams = SearchParams.safeParse({
     name: search.get("name") ?? undefined,
     period: search.get("period") ?? undefined,
     from: search.get("from") ?? undefined,
     to: search.get("to") ?? undefined,
   });
+  const normalizedName =
+    parsedSearchParams.success
+      ? (parsedSearchParams.data.name?.trim() || undefined)
+      : undefined;

Then pass normalizedName below:

-    name: parsedSearchParams.data.name,
+    name: normalizedName,

65-66: Return JSON response for consistency

Use Remix json() helper for proper headers and consistency with other loaders.

-import { type LoaderFunctionArgs } from "@remix-run/server-runtime";
+import { json, type LoaderFunctionArgs } from "@remix-run/server-runtime";
...
-  return result;
+  return json(result);

12-17: Optional: guard against NaN for from/to

If arbitrary strings hit from/to, coerce can yield NaN. It currently falls back via timeFilters, but you can explicitly reject NaN for cleaner semantics.

-const SearchParams = z.object({
-  name: z.string().optional(),
-  period: z.preprocess((value) => (value === "all" ? undefined : value), z.string().optional()),
-  from: z.coerce.number().optional(),
-  to: z.coerce.number().optional(),
-});
+const SearchParams = z.object({
+  name: z.string().optional(),
+  period: z.preprocess((v) => (v === "all" ? undefined : v), z.string().optional()),
+  from: z.coerce.number().refine(Number.isFinite, "Invalid 'from'").optional(),
+  to: z.coerce.number().refine(Number.isFinite, "Invalid 'to'").optional(),
+});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7135b71 and e3d61a8.

📒 Files selected for processing (4)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (8 hunks)
  • apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx (1 hunks)
  • apps/webapp/app/hooks/useSearchParam.ts (1 hunks)
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx
  • apps/webapp/app/hooks/useSearchParam.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx
  • apps/webapp/app/hooks/useSearchParam.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx
  • apps/webapp/app/hooks/useSearchParam.ts
  • apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/hooks/useSearchParam.ts
🧠 Learnings (1)
📚 Learning: 2025-07-12T18:00:06.163Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/utils/searchParams.ts:16-18
Timestamp: 2025-07-12T18:00:06.163Z
Learning: The `objectToSearchParams` function in `apps/webapp/app/utils/searchParams.ts` is used to generate URL parameters from objects and is separate from code that parses incoming search parameters. Changes to this function only affect places where it's used to create URLs, not places that parse search parameters from external sources.

Applied to files:

  • apps/webapp/app/hooks/useSearchParam.ts
🧬 Code graph analysis (1)
apps/webapp/app/routes/resources.environments.$envId.runs.tags.tsx (4)
apps/webapp/app/services/session.server.ts (1)
  • requireUserId (25-35)
apps/webapp/app/db.server.ts (1)
  • $replica (103-106)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
apps/webapp/app/presenters/v3/RunTagListPresenter.server.ts (1)
  • RunTagListPresenter (26-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/components/runs/v3/WaitpointTokenFilters.tsx (1)

333-333: Relying on URLSearchParams encoding is correct

Setting the raw searchValue is fine; URLSearchParams will encode on toString. No double-encoding issues.

apps/webapp/app/hooks/useSearchParam.ts (1)

36-36: Good removal of manual decoding

Returning search.get()/getAll() directly is correct—URLSearchParams already decodes percent-encoding. This avoids double-decoding and unexpected behavior.

Also applies to: 44-44

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3d61a8 and a33533d.

📒 Files selected for processing (1)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
  • useEnvironment (19-23)
apps/webapp/app/hooks/useSearchParam.ts (1)
  • useSearchParams (7-64)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)

825-829: Guard sending period='all' (verify server expectations)

If the UI ever sets period='all', this forwards it to the loader. Confirm the loader treats 'all' as unbounded; otherwise skip sending the period param when it’s 'all'.

Proposed defensive tweak:

-    if (period) {
-      searchParams.set("period", period);
-    }
+    if (period && period !== "all") {
+      searchParams.set("period", period);
+    }

Based on learnings

Also applies to: 841-849


971-983: Use the debounced value ‘s’ consistently

Inside the debounced callback you conditionally check searchValue but set the query from s. Use s for both.

-      if (searchValue) {
-        searchParams.set("query", s);
-      }
+      if (s) {
+        searchParams.set("query", s);
+      }

1233-1244: Use the debounced value ‘s’ consistently

Same here: prefer checking s instead of the outer searchValue.

-      if (searchValue) {
-        searchParams.set("query", s);
-      }
+      if (s) {
+        searchParams.set("query", s);
+      }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e3d61a8 and a33533d.

📒 Files selected for processing (1)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
  • useEnvironment (19-23)
apps/webapp/app/hooks/useSearchParam.ts (1)
  • useSearchParams (7-64)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
🔇 Additional comments (4)
apps/webapp/app/components/runs/v3/RunFilters.tsx (4)

61-61: Typesafe loader import LGTM

Importing the env-scoped tags loader and typing useFetcher off it is correct.


64-64: Shared time filters import LGTM

Bringing in TimeFilter/timeFilters aligns this component with the new time range model.


279-287: Tags parsing from search params LGTM

Using URLSearchParams.getAll directly (no manual decode) is fine and consistent with the custom hook behavior.


856-866: Tag list construction LGTM

Prepending selected tags when the search is empty, merging with fetched tags, deduping, and sorting via matchSorter is sound.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (1)

813-851: Previous review concern resolved: environment.id now in dependencies.

The useEffect dependency array at line 851 now correctly includes environment.id, resolving the issue flagged in previous reviews where switching environments wouldn't trigger a tag refetch. The implementation properly includes all necessary dependencies:

  • environment.id: Ensures refetch when environment changes ✓
  • searchValue: Triggers immediate search (no debounce) ✓
  • period, from?.getTime(), to?.getTime(): Time filter changes trigger refetch ✓

Optional: Consider debouncing search for consistency.

Unlike QueuesDropdown (line 970) and VersionsDropdown (line 1233), which use useDebounceEffect with a 250ms delay for search-driven fetches, TagsDropdown triggers an immediate fetch on every searchValue change. For consistency and to reduce request volume during rapid typing, consider applying the same debouncing pattern.

Apply this pattern for debounced search (keeping immediate fetches for time/env changes):

- useEffect(() => {
+ useDebounceEffect(
+   searchValue,
+   (s) => {
      const searchParams = new URLSearchParams();
-     if (searchValue) {
+     if (s) {
-       searchParams.set("name", searchValue);
+       searchParams.set("name", s);
      }
      if (period) {
        searchParams.set("period", period);
      }
      if (from) {
        searchParams.set("from", from.getTime().toString());
      }
      if (to) {
        searchParams.set("to", to.getTime().toString());
      }
      fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
+   },
+   250
+ );
+ 
+ // Separate immediate effect for time/env changes
+ useEffect(() => {
+   if (searchValue) return; // Skip if user is actively searching
+   const searchParams = new URLSearchParams();
+   if (period) {
+     searchParams.set("period", period);
+   }
+   if (from) {
+     searchParams.set("from", from.getTime().toString());
+   }
+   if (to) {
+     searchParams.set("to", to.getTime().toString());
+   }
+   fetcher.load(`/resources/environments/${environment.id}/runs/tags?${searchParams}`);
  }, [environment.id, searchValue, period, from?.getTime(), to?.getTime()]);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a33533d and e6727c7.

📒 Files selected for processing (1)
  • apps/webapp/app/components/runs/v3/RunFilters.tsx (7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/components/runs/v3/RunFilters.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/runs/v3/RunFilters.tsx (3)
apps/webapp/app/hooks/useEnvironment.tsx (1)
  • useEnvironment (19-23)
apps/webapp/app/hooks/useSearchParam.ts (1)
  • useSearchParams (7-64)
apps/webapp/app/components/runs/v3/SharedFilters.tsx (1)
  • timeFilters (106-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/components/runs/v3/RunFilters.tsx (2)

61-61: LGTM! Environment-scoped loader and time filters properly integrated.

The import changes correctly reflect the shift from project-scoped to environment-scoped tag fetching, and the timeFilters helper properly normalizes the time range parameters for the tag fetch request.

Also applies to: 64-64, 825-829


853-866: LGTM! Tag list construction and memoization are correct.

The filtered tags logic properly:

  • Preserves selected tags when search is empty (line 856)
  • Merges fetched tags from the loader (line 863)
  • Deduplicates using Set (line 865)
  • Includes appropriate memo dependencies (line 866)

@matt-aitken matt-aitken merged commit 885d2d3 into main Oct 15, 2025
31 checks passed
@matt-aitken matt-aitken deleted the tags-list-performance branch October 15, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants